Fendermint [247, 248, 249]: Checkpoint signatures #202

dnkolegov · 2023-09-15T18:23:33Z

This PR addresses

Create Checkpoint actor to accumulate signatures ipc#207 - Checkpointing actor
Method in Checkpoint actor to add new unsigned checkpoint ipc#206 - Checkpoint creation
Method in Checkpoint actor to add new validator signature ipc#205 - Checkpoint signatures

aakoshh · 2023-09-18T09:52:11Z

src/gateway/GatewayRouterFacet.sol

+    event SignaturesUpdated(uint256 h, bytes32 pof, Membership membership, bytes signature);
+    event SignatureInvalid(uint256 h, bytes32 pof, Membership membership, bytes signature);
+    event PoFThresholdReached(uint256 h, bytes32 pof, Membership membership, uint256 threshold);


What are h and pof? If h is block height, shouldn't it be uint64? Does it cost anything to have longer field names?

A rule of thumb was to make field names which are long lived more verbose, and only use single letters for short-lived local variables.

for solidity, we have a convention to use U256 for block number.

@cryptoAtwill but we use uint64 in all mappings mapping block numbers

Yeah, that's the thing. In fvm, it's either u64 or i64, but in solidity, it's U256. How we do this previously is just use U256 in solidity as it's the convention. Then in rust, we convert to whatever is the preferred type. I don't feel we will run into overflow though. But feel free to suggest otherwise.

It sounds like just a dogmatic choice that is a forever thorn in one's side of how to handle the potential conversion errors, and takes up more space, but if that's how you want it 🤷

but in solidity, it's U256

@cryptoAtwill In the Solidity actors we also use uint64 as far as I see

src/gateway/GatewayRouterFacet.sol

aakoshh · 2023-09-18T09:58:38Z

src/gateway/GatewayRouterFacet.sol

+
+        if (signerExists) {
+            uint256 threshold = s.bottomUpCollectedSignaturesThreshold[h][pof];
+            threshold += membership.validators[signerIndex].weight;


Does this need to be written back to the storage?

I'm sorry what was the fix? I'm not a Solidity storage buff; it looks like you can take records and modify them and expect them to be persisted, I'm just checking if that's the case.

The fix is to use "storage" type instead of "memory" type. But it was not fixed actually in that sense. Fixed now. Thank you.

src/gateway/GatewayRouterFacet.sol

src/lib/LibGatewayActorStorage.sol

src/gateway/GatewayRouterFacet.sol

src/structs/Checkpoint.sol

src/lib/LibGatewayActorStorage.sol

aakoshh

This looks good now, just minor comments and questions 👍

src/gateway/GatewayRouterFacet.sol

dnkolegov · 2023-09-19T20:16:20Z

This looks good now, just minor comments and questions 👍

Great catches!
Thank you @aakoshh !

cryptoAtwill · 2023-09-20T02:19:57Z

src/gateway/GatewayRouterFacet.sol

+            revert CheckpointMembershipNotCreated();
+        }
+
+        bytes32 checkpointHash = checkpoint.toHash();


is there a difference btw checkpointHash and checkpointInfo.hash?

If the checkpoint is read from storage, that means we can storage the checkpointHash in storage as well? So that we dont have to compute it every time.

I forgot checnge that one. Thanks!

cryptoAtwill · 2023-09-20T02:20:54Z

src/gateway/GatewayRouterFacet.sol

+        uint256 weight,
+        bytes memory signature
+    ) external {
+        BottomUpCheckpointNew memory checkpoint = s.bottomUpCheckpoints[height];


do we do backward check? Like the height should not have already committed?

The current idea is to continue to allow accumulating signatures even if it has been committed. In this case, we update signatures and emit an event with updated weight. If I understood you right...

aakoshh

Bullseye 🎯

src/structs/Checkpoint.sol

adlrocha · 2023-09-26T14:41:06Z

src/structs/Checkpoint.sol

@@ -21,6 +21,39 @@ struct BottomUpCheckpoint {
    bytes proof;
 }

+/// @notice A bottom-up checkpoint type.
+// TODO: Remove old BottomUpCheckpoint, rename BottomUpCheckpointNew to BottomUpCheckpoint and update the codebase.


@dnkolegov, @cryptoAtwill, are we tracking this TODO somewhere?

https://github.com/consensus-shipyard/ipc-solidity-actors/issues/210

adlrocha · 2023-09-26T14:41:39Z

src/gateway/GatewayGetterFacet.sol

@@ -50,8 +50,9 @@ contract GatewayGetterFacet {
        return s.networkName;
    }

+    // TODO: remove or add a new getter


@dnkolegov, should we track this in an issue like the others TODO?

https://github.com/consensus-shipyard/ipc-solidity-actors/issues/210

start signature accumulator

799d8bf

dnkolegov marked this pull request as draft September 15, 2023 18:23

dnkolegov added 2 commits September 16, 2023 15:50

draft interfaces

d3381c8

process signatures

7425268

aakoshh changed the base branch from dev to membership September 18, 2023 09:50

rename vars

6f4c1b3

aakoshh reviewed Sep 18, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 18, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 18, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 18, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 18, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 18, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 18, 2023

View reviewed changes

src/lib/LibGatewayActorStorage.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 18, 2023

View reviewed changes

src/lib/LibGatewayActorStorage.sol Outdated Show resolved Hide resolved

dnkolegov added 2 commits September 18, 2023 18:08

fix comments

5df4cf3

fix comments

7f64858

dnkolegov requested a review from aakoshh September 18, 2023 21:28

cryptoAtwill reviewed Sep 19, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 19, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 19, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 19, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 19, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 19, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh changed the title ~~Fendermint issue 247: signatures~~ Fendermint issue 249: signatures Sep 19, 2023

drafting new impl

ce58204

aakoshh reviewed Sep 19, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 19, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 19, 2023

View reviewed changes

src/structs/Checkpoint.sol Show resolved Hide resolved

aakoshh reviewed Sep 19, 2023

View reviewed changes

src/lib/LibGatewayActorStorage.sol Outdated Show resolved Hide resolved

aakoshh reviewed Sep 19, 2023

View reviewed changes

src/gateway/GatewayRouterFacet.sol Outdated Show resolved Hide resolved

fix comments

f232eb1

dnkolegov requested review from aakoshh and cryptoAtwill September 19, 2023 20:19

cryptoAtwill reviewed Sep 20, 2023

View reviewed changes

use stored hash

f9402ae

aakoshh approved these changes Sep 20, 2023

View reviewed changes

dnkolegov added 3 commits September 20, 2023 13:34

fix proof validation and storage type

c788bef

interface test

e3a390a

tests

f7ed692

dnkolegov marked this pull request as ready for review September 20, 2023 18:31

aakoshh changed the title ~~Fendermint issue 249: signatures~~ Fendermint [247, 248, 249]: Checkpoint signatures Sep 21, 2023

more tests

083109d

aakoshh mentioned this pull request Sep 21, 2023

FM-254: Create checkpoint consensus-shipyard/fendermint#273

Merged

6 tasks

dnkolegov changed the base branch from membership to dev September 21, 2023 10:43

fix error

4190c00

dnkolegov changed the base branch from dev to membership September 21, 2023 11:43

dnkolegov merged commit c3c26dc into membership Sep 21, 2023

dnkolegov deleted the fendermint247 branch September 21, 2023 11:44

dnkolegov mentioned this pull request Sep 21, 2023

Method in Checkpoint actor to add new unsigned checkpoint consensus-shipyard/ipc#206

Closed

aakoshh reviewed Sep 21, 2023

View reviewed changes

src/structs/Checkpoint.sol Show resolved Hide resolved

dnkolegov mentioned this pull request Sep 21, 2023

(Optional) Method in Checkpoint actor to check for quorum and emit signature consensus-shipyard/ipc#306

Closed

adlrocha reviewed Sep 26, 2023

View reviewed changes

aakoshh mentioned this pull request Sep 19, 2023

Method in Checkpoint actor to add new validator signature consensus-shipyard/ipc#205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fendermint [247, 248, 249]: Checkpoint signatures #202

Fendermint [247, 248, 249]: Checkpoint signatures #202

dnkolegov commented Sep 15, 2023 •

edited by aakoshh

Loading

aakoshh Sep 18, 2023

cryptoAtwill Sep 18, 2023

dnkolegov Sep 18, 2023

cryptoAtwill Sep 19, 2023

aakoshh Sep 19, 2023

dnkolegov Sep 19, 2023 •

edited

Loading

aakoshh Sep 18, 2023

dnkolegov Sep 19, 2023

aakoshh Sep 20, 2023

dnkolegov Sep 20, 2023

aakoshh left a comment

dnkolegov commented Sep 19, 2023

cryptoAtwill Sep 20, 2023

cryptoAtwill Sep 20, 2023

dnkolegov Sep 20, 2023

cryptoAtwill Sep 20, 2023

dnkolegov Sep 20, 2023

aakoshh left a comment

adlrocha Sep 26, 2023

dnkolegov Sep 26, 2023

adlrocha Sep 26, 2023

dnkolegov Sep 26, 2023

Fendermint [247, 248, 249]: Checkpoint signatures #202

Fendermint [247, 248, 249]: Checkpoint signatures #202

Conversation

dnkolegov commented Sep 15, 2023 • edited by aakoshh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnkolegov Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh left a comment

Choose a reason for hiding this comment

dnkolegov commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnkolegov commented Sep 15, 2023 •

edited by aakoshh

Loading

dnkolegov Sep 19, 2023 •

edited

Loading